s3: Filter S3 profiles to only those used by the application - #503
s3: Filter S3 profiles to only those used by the application#503parikshithb wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughS3 discovery now reads application profile references from the primary VRG. It derives prefixes from the VRG namespace and DRPC name. Gathering, validation, and failed-test reporting filter S3 secrets to application-associated profiles. ChangesApplication-specific S3 discovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ApplicationCommand
participant DRPC
participant PrimaryVRG
participant S3Profiles
participant S3Secrets
ApplicationCommand->>DRPC: Read application DRPC
ApplicationCommand->>PrimaryVRG: Resolve VRG namespace and primary cluster
ApplicationCommand->>S3Profiles: Read referenced profile names
ApplicationCommand->>S3Secrets: Retrieve matching S3 secrets
ApplicationCommand->>ApplicationCommand: Derive prefix from VRG namespace and DRPC name
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/gather/command.go`:
- Around line 406-408: In pkg/gather/command.go lines 406-408, validate that
every appProfiles entry has a matching hub profile before filtering and fail the
inspection when any are missing. Apply the same pre-gather validation in
pkg/validate/application/command.go lines 312-314. In pkg/test/command.go lines
267-269, log the missing profile names and skip or mark the affected
application’s S3 gathering as incomplete so gathering cannot proceed with an
empty profile list.
In `@pkg/test/command.go`:
- Around line 490-491: Update the profile and prefix collection around
appProfiles and prefixes so each application’s profiles remain associated with
its own ramen.ApplicationS3Prefix value instead of building independent unions.
Adjust the data passed to s3.Gather to preserve these per-application
associations and prevent profiles from receiving unrelated prefixes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e658c3db-a917-4316-b891-2283456f9c32
📒 Files selected for processing (4)
pkg/gather/command.gopkg/ramen/ramen.gopkg/test/command.gopkg/validate/application/command.go
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Build (linux, arm64)
- GitHub Check: Build (windows, amd64)
- GitHub Check: Test (ubuntu-24.04)
- GitHub Check: Build (darwin, amd64)
- GitHub Check: Build (linux, amd64)
- GitHub Check: Test (macos-15)
- GitHub Check: Go compatibility
- GitHub Check: Build (darwin, arm64)
- GitHub Check: Test (ubuntu-24.04-arm)
- GitHub Check: Test (macos-15-intel)
- GitHub Check: Lint
- GitHub Check: Test (windows-latest)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
**/*.go: Use proper punctuation in comments - end sentences with periods
All Go files need SPDX license headers - check existing files for the format
Check existing code for error formatting conventions in Go
Files:
pkg/test/command.gopkg/ramen/ramen.gopkg/validate/application/command.gopkg/gather/command.go
🔇 Additional comments (4)
pkg/ramen/ramen.go (1)
375-391: LGTM!pkg/gather/command.go (1)
364-396: LGTM!Also applies to: 420-420
pkg/validate/application/command.go (1)
270-302: LGTM!Also applies to: 326-326
pkg/test/command.go (1)
254-256: LGTM!Also applies to: 448-488, 492-494
|
|
||
| func (c *Command) s3PrefixesToGather(reader gathering.OutputReader) []string { | ||
| // s3InfoToGather returns the S3 profile names and prefixes for failed tests, | ||
| // reading the DRPC and VRG once per test. |
There was a problem hiding this comment.
Keep the previous code - the only difference is the profiles used to gather. The old code gathered from all profiles, we need to gather only from the application profiles for each application.
The simplest way is to do the gather per failed application instead of single gather for all applications:
for app in failed appes:
get the prefix for tha app
get the profiles for the app
gather s3 data for profiles and prefix
But since we use the same drpolicy for all test apps - we will have the same profiles names for every app - so we can do:
get prefixes for all apps
get profiles for all apps (removing duplicates)
gather s3 data from profiles and prefixes
9cb325b to
6f903e3
Compare
nirs
left a comment
There was a problem hiding this comment.
Need deeper review, added few comments for now.
| storeProfiles, err := ramen.ClusterProfiles(reader, configMapName, configMapNamespace) | ||
| storeProfiles, err := ramen.ApplicationProfiles( | ||
| c, reader, c.outputReader, configMapName, configMapNamespace, | ||
| c.opts.DRPCName, c.opts.DRPCNamespace) |
There was a problem hiding this comment.
This is a good start - change the function we call to get the application profiles instead of all profiles!
But we pass 2 many arguments like configMapName and configMapNamespace - this values comes from ramen package, so ramen already know them, so no need to pass them.
Need rethink passing these arguments:
- c - the command - is it for the logger? we should pass the logger instead.
- reader
- c.outputReader - would be better to implement this in the ramen package and not pass functions like this
- configMapName, configMapNamespace - ramen already know these.
There was a problem hiding this comment.
- ctx is used not for logging. It's needed currently for
PrimaryCluster(ctx, drpc)which callsctx.Env().GetCluster() - c.outputReader: not sure how, add OutputReader(string) to the ramen.Context interface?
- configmap:
- configmap Name: Yes, but existing
configMapName := ramen.HubOperatorConfigMapNameis passed from caller, refactor this code to directly use? - Configmap ns: this comes from user config
config.Namespaces.RamenHubNamespacevaries between k8s and ocp
- configmap Name: Yes, but existing
There was a problem hiding this comment.
ctx is good - matching how we pass commands everywhere - keep it.
The hub reader is already used in other functions, but current coded needed single cluster, and here we need to read form 2 clusters. Reading clusters requires the command dataDir(), so we added outputDir() since it was used only in the command command. Passing it to the ramen code works but not great.
If we add OutputReader(string) to the context interface, ramen code can create readers from the context so we don't need to pass the hub and the outputReader function. This seems useful since ramen package need to read stuff from the output directory.
The ramen package knows the ramen hub config map name so we don't need to pass it.
The context has the config, so the ramen package can find the config namespace.
With these changes the call will become:
storeProfiles, err := ramen.ApplicationProfiles(c, c.opts.DRPCName, c.opts.DRPCNamespace)Lets try do this:
- commit 1: Add OuptutReader to ramen context
- commit 2: Add ApplicationS3Profiles (this commit)
If adding OutputReader() is too big we can pass the function and let ramen.ApplicationS3Profiles create the readers.
We can change ramen.ApplicationS3Prefix() to accept a context later for creating the reader instead of passing the reader. This can be a followup PR in main. release 0.23 will have less consistent code.
|
@parikshithb Please link to the downstream bug so we have easy way to find it, or create a new upstream issue linking to this bug. |
When a hub is shared by multiple managed clusters, the ramen configmap contains S3 profiles for all DR pairs. Previously, validate and gather commands processed all profiles from the configmap, causing failures for profiles unrelated to the application being validated. Add ramen.ApplicationProfiles() that reads the primary VRG's spec.s3Profiles and filters the configmap profiles to only those referenced by the application. This replaces the ClusterProfiles() call in both validate application and gather application commands. Assisted-by: Opus 4.6 Signed-off-by: Parikshith <parikshithb@gmail.com>
6f903e3 to
f1fbf4a
Compare
Problem
When a hub is shared by multiple managed clusters, validate and gather
application commands access all S3 profiles from the hub configmap,
causing failures for profiles unrelated to the application.
Solution
Add
ramen.ApplicationProfiles()that filters hub configmap profiles toonly those referenced by the primary VRG's
spec.s3Profiles. One-linechange in each caller to use it instead of
ClusterProfiles().Fixes DFBUGS-9708